Fix Jailbreak — bind the CR 110.2a "under their control" clause (#6691) - #6814
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe parser now recognizes battlefield-entry control clauses, preserves explicit, default, and unbound states, and binds clauses to player context. Zone-change lowering fails closed for unresolved possessors. Parser and integration tests cover control binding and Jailbreak. ChangesEnters-under control binding
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CardText
participant ReturnDestinationParser
participant ControlClauseBinder
participant ZoneChangeLowering
participant GameEffect
CardText->>ReturnDestinationParser: parse battlefield destination and control clause
ReturnDestinationParser->>ControlClauseBinder: pass possessor and antecedent context
ControlClauseBinder->>ZoneChangeLowering: return EntersUnderSpec
ZoneChangeLowering->>GameEffect: create controller override or unimplemented anaphor effect
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/engine/src/parser/oracle_nom/enters_under.rs (1)
325-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe final
(p, _)arm defeats the exhaustiveness discipline this module relies on elsewhere.
map_relative_player_scopeis deliberately wildcard-free so "adding aControllerRefvariant breaks the build here" (Line 246).bind_control_clausegives up that property: the deferred possessor forms listed at Lines 150-160 ("under an opponent's control","under target player's control", …) will, the moment they are added asControlClausePossessorvariants, fall silently intoUnboundAnaphorrather than forcing a decision at this binding table. Fail-closed is the safe direction, but the card becomes an unexplained coverage gap instead of a compile error.Match the possessor axis explicitly so the compiler flags a new variant.
As per coding guidelines: "Use exhaustive
matchexpressions without wildcard fallbacks when matching known enums, allowing the compiler to detect missing variants."♻️ Explicit anaphor arms instead of the tuple wildcard
- // Fail closed: no nameable antecedent (and, for the demonstrative, an - // object-owner antecedent it may not legally use). - (p, _) => EntersUnderSpec::UnboundAnaphor(p), + // Fail closed: no nameable antecedent (and, for the demonstrative, an + // object-owner antecedent it may not legally use). Enumerated so a new + // possessor variant breaks the build here rather than becoming a silent + // coverage gap. + ( + p @ (ControlClausePossessor::TheirAnaphor + | ControlClausePossessor::ThatPlayerDemonstrative), + ControlAnaphorAntecedent::MovedObjectOwner | ControlAnaphorAntecedent::Unnameable, + ) => EntersUnderSpec::UnboundAnaphor(p),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_nom/enters_under.rs` around lines 325 - 354, Update bind_control_clause to remove the tuple wildcard `(p, _)` and enumerate each currently unsupported ControlClausePossessor variant explicitly, returning UnboundAnaphor for those cases. Preserve the existing You, Owner, TheirAnaphor, and ThatPlayerDemonstrative behavior while making the match exhaustive so adding a new possessor variant produces a compile-time error.Source: Coding guidelines
crates/engine/src/parser/oracle_effect/sequence.rs (1)
5308-5326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe searched player's filter is available here and would license the CR 108.3 owner antecedent.
Passing
Noneasmoved_objectis accurate for this AST shape, but the information is not actually absent: the precedingEffect::SearchLibraryindefscarries the player filter whose library is searched, and for "search target opponent's library … put it onto the battlefield under their control" that player is the found card's owner (CR 400.3 — a card in a library is in its owner's library). As written, such a card can only reachContextPlayeror fail closed. Not a defect in this PR's scope, but worth a note here (or in the deferred list) so the seam's gap is attributable rather than looking like an inherent limitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/sequence.rs` around lines 5308 - 5326, Document this deferred limitation at the bind_control_clause seam: although moved_object is correctly None for this AST shape, the preceding Effect::SearchLibrary in defs provides the searched player filter, which can identify the found card’s owner for CR 108.3. Add a concise note here or to the deferred-work list explaining that opponent-library searches currently cannot use this owner antecedent and may resolve only to ContextPlayer or fail closed.crates/engine/src/parser/oracle_ir/ast.rs (1)
220-271: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThree-state contract is only half-adopted across
enters_underfields.
SearchDestination,ReturnToBattlefield,ReturnAllToZone,ZoneChange, andZoneChangeAllnow carryEntersUnderSpec, but the sibling controller-override fields in this same file stayOption<ControllerRef>:
ContinuationAst::DigFromAmong.enters_under(Line 406)ContinuationAst::RevealUntilKept.enters_under(Line 469)PutImperativeAst::Manifest.enters_under(Line 1511)Those three are fed by literal
scan_contains(lower, "under your control")checks inoracle_effect/sequence.rs, so a printed"under their control"/"under that player's control"at those seams still degrades to the CR 110.2 default rather than failing closed — exactly the class this PR is closing. Today no printed card appears to reach them with a third-person clause, so this is a coverage/uniformity gap rather than an active misparse; worth either migrating them ontoEntersUnderSpecin a follow-up or recording the carve-out in this doc comment so the asymmetry is intentional and discoverable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_ir/ast.rs` around lines 220 - 271, Complete the three-state contract for the remaining entry-controller fields: change ContinuationAst::DigFromAmong.enters_under, ContinuationAst::RevealUntilKept.enters_under, and PutImperativeAst::Manifest.enters_under from Option<ControllerRef> to EntersUnderSpec, and update their sequence-lowering/parsing paths to preserve unbound third-person control clauses as UnboundAnaphor instead of defaulting. Reuse EntersUnderSpec’s existing helpers and ensure all constructors and consumers handle Default, Override, and UnboundAnaphor consistently.crates/engine/src/parser/oracle_effect/tests.rs (1)
29864-29992: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a destination-layer unit test for the third-person control-clause fallback.
jailbreak_enters_under_their_control_binds_to_the_ownerandunbindable_anaphor_fails_closed_instead_of_defaultingexercise the newTheirAnaphor/ThatPlayerDemonstrativefallback only through fullparse_effect_chain/parse_oracle_text, conflating the destination-parsing layer (strip_return_destination_ext_with_remainderinlower.rs) with the downstream binding layer. A direct test mirroringreturn_destination_owners_control_not_under_your_control— e.g. assertingstrip_return_destination_ext("it to the battlefield under their control").1.unwrap().control == Some(ControlClausePossessor::TheirAnaphor)— would isolate a regression in the newparse_leading_control_clausefallback from a regression inbind_control_clause.🧪 Suggested additional test
#[test] fn return_destination_under_their_control_recognized_as_anaphor() { let (_, dest) = strip_return_destination_ext("it to the battlefield under their control"); let d = dest.expect("should parse destination"); assert_eq!(d.control, Some(ControlClausePossessor::TheirAnaphor)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/tests.rs` around lines 29864 - 29992, Add a focused destination-layer unit test alongside return-destination tests, using strip_return_destination_ext (or its remainder variant) to parse “it to the battlefield under their control” and assert the destination control is Some(ControlClausePossessor::TheirAnaphor). Keep this test independent of parse_effect_chain and bind_control_clause so it specifically covers parse_leading_control_clause’s fallback recognition.crates/engine/src/parser/oracle_effect/imperative.rs (1)
2331-2336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the fail-closed guard into one authority on
EntersUnderSpec. The identicalunbound_possessor()→Effect::unimplemented("change_zone_enters_under_anaphor", p.printed_clause())block, including the gap-name string literal, is repeated at five lowering sites. A single accessor (e.g.EntersUnderSpec::unimplemented_if_unbound(&self) -> Option<Effect>) keeps the gap name and CR 110.2a rationale in one place and makes a future lowering site that forgets the guard obvious.
crates/engine/src/parser/oracle_effect/imperative.rs#L2331-L2336: replace with the shared accessor (if let Some(e) = enters_under.unimplemented_if_unbound() { return e; }).crates/engine/src/parser/oracle_effect/imperative.rs#L2396-L2401: same replacement in theReturnAllToZonearm.crates/engine/src/parser/oracle_effect/imperative.rs#L6762-L6767: same replacement in thePutImperativeAst::ZoneChangeAllarm.crates/engine/src/parser/oracle_effect/imperative.rs#L6796-L6801: same replacement in thePutImperativeAst::ZoneChangearm.crates/engine/src/parser/oracle_effect/imperative.rs#L11635-L11640: same, wrapping the returned effect inparsed_clause(..).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/imperative.rs` around lines 2331 - 2336, Centralize the unbound-possessor fail-closed behavior in an accessor on EntersUnderSpec, such as unimplemented_if_unbound, preserving the change_zone_enters_under_anaphor gap name and printed clause. Replace the duplicated guards at crates/engine/src/parser/oracle_effect/imperative.rs lines 2331-2336, 2396-2401, 6762-6767, and 6796-6801 with the accessor and return its Effect; at lines 11635-11640, use the accessor and wrap the returned effect in parsed_clause as currently required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_effect/sequence.rs`:
- Around line 3727-3741: Handle the unbound enters_under.possessor() case as a
failed assembly: at
crates/engine/src/parser/oracle_effect/sequence.rs:3727-3741, stop or neutralize
the existing library-to-battlefield ChangeZone in previous before
apply_search_destination_to_ability_chain so no executable wrong-controller move
remains; at crates/engine/src/parser/oracle_effect/sequence.rs:3770-3781, skip
forward_result and Effect::Attach wiring when put_effect is the
Effect::unimplemented fragment, or fold the attach behavior into that fragment.
In `@crates/engine/tests/integration/issue_6691_enters_under_their_control.rs`:
- Around line 59-143: Add a separate integration test using a production card
ability with a named-player controller binding, such as The Beamtown Bullies’
“that player’s control,” where the default controller differs from the
referenced player. Build and resolve the scenario through the normal card
database and game runner pipeline, then assert the permanent enters under the
named player’s control and that the relevant effect does not revert.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 2331-2336: Centralize the unbound-possessor fail-closed behavior
in an accessor on EntersUnderSpec, such as unimplemented_if_unbound, preserving
the change_zone_enters_under_anaphor gap name and printed clause. Replace the
duplicated guards at crates/engine/src/parser/oracle_effect/imperative.rs lines
2331-2336, 2396-2401, 6762-6767, and 6796-6801 with the accessor and return its
Effect; at lines 11635-11640, use the accessor and wrap the returned effect in
parsed_clause as currently required.
In `@crates/engine/src/parser/oracle_effect/sequence.rs`:
- Around line 5308-5326: Document this deferred limitation at the
bind_control_clause seam: although moved_object is correctly None for this AST
shape, the preceding Effect::SearchLibrary in defs provides the searched player
filter, which can identify the found card’s owner for CR 108.3. Add a concise
note here or to the deferred-work list explaining that opponent-library searches
currently cannot use this owner antecedent and may resolve only to ContextPlayer
or fail closed.
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 29864-29992: Add a focused destination-layer unit test alongside
return-destination tests, using strip_return_destination_ext (or its remainder
variant) to parse “it to the battlefield under their control” and assert the
destination control is Some(ControlClausePossessor::TheirAnaphor). Keep this
test independent of parse_effect_chain and bind_control_clause so it
specifically covers parse_leading_control_clause’s fallback recognition.
In `@crates/engine/src/parser/oracle_ir/ast.rs`:
- Around line 220-271: Complete the three-state contract for the remaining
entry-controller fields: change ContinuationAst::DigFromAmong.enters_under,
ContinuationAst::RevealUntilKept.enters_under, and
PutImperativeAst::Manifest.enters_under from Option<ControllerRef> to
EntersUnderSpec, and update their sequence-lowering/parsing paths to preserve
unbound third-person control clauses as UnboundAnaphor instead of defaulting.
Reuse EntersUnderSpec’s existing helpers and ensure all constructors and
consumers handle Default, Override, and UnboundAnaphor consistently.
In `@crates/engine/src/parser/oracle_nom/enters_under.rs`:
- Around line 325-354: Update bind_control_clause to remove the tuple wildcard
`(p, _)` and enumerate each currently unsupported ControlClausePossessor variant
explicitly, returning UnboundAnaphor for those cases. Preserve the existing You,
Owner, TheirAnaphor, and ThatPlayerDemonstrative behavior while making the match
exhaustive so adding a new possessor variant produces a compile-time error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 416d9312-f59c-4081-bf6b-12ba184477f1
⛔ Files ignored due to path filters (2)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__boseiju_who_endures_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__case_of_the_stashed_skeleton_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (11)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_nom/enters_under.rscrates/engine/src/parser/oracle_nom/mod.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/issue_6691_enters_under_their_control.rscrates/engine/tests/integration/main.rs
|
Maintainer conflict port for |
Parse changes introduced by this PR · 6 card(s), 8 signature(s) (baseline: main
|
|
Maintainer CI repair is pushed on current head |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/engine/src/parser/oracle_effect/lower.rs (2)
6754-6768: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConsume battlefield riders after the control clause.
After
parse_leading_control_clausesucceeds, this path immediately returns after consuming one space. Valid forms such asto the battlefield under their control face down and tappedtherefore leave the riders in the remainder and returnface_down: false/enter_tapped: false. The other destination path already scans riders after control; reuse that logic here.
As per path instructions, engine destination parsing must remain composable and rules-correct across rider orderings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/lower.rs` around lines 6754 - 6768, Update the battlefield return path following parse_leading_control_clause to consume and apply trailing battlefield riders such as face down and tapped, including valid rider orderings, before constructing ReturnDestination. Reuse the existing rider-scanning logic from the other destination path, preserving the remaining input and setting face_down and enter_tapped correctly.Source: Path instructions
6639-6662: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve control clauses after enter-with-counters suffixes.
Control parsing only runs before
parse_with_counters_suffix_spanned. For valid text such aswith two stun counters under their control, the counter parser consumes the prefix, while the trailing control clause is neither bound nor returned; the destination silently falls back tocontrol: None, breaking CR 110.2a behavior. Parse the counter-suffix remainder for a control clause, and add an assertion fordest.controlto the existing regression fixture.
As per path instructions, engine parsing must preserve rules-correct control binding across valid clause orderings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/lower.rs` around lines 6639 - 6662, Update the destination parsing flow around parse_with_counters_suffix_spanned to inspect its remaining text for a trailing control clause before finalizing the destination. Use parse_leading_control_clause to bind the parsed player to dest.control while preserving existing rider and suffix handling for orderings such as counters followed by control. Extend the existing regression fixture with an assertion that dest.control is populated.Source: Path instructions
crates/engine/src/parser/oracle_effect/imperative.rs (1)
2052-2057: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCR citation mismatch: "CR 110.1" doesn't describe controller ownership.
CR 110.1 defines what a permanent is ("A permanent is a card or token on the battlefield") and says nothing about controllers. The claim "only permanents have a controller" maps to CR 110.2 ("Every permanent has a controller") together with CR 108.4 ("A card doesn't have a controller unless that card represents a permanent or spell"), not CR 110.1.
📝 Suggested annotation fix
- // CR 110.1 (docs/MagicCompRules.txt:614): only - // permanents have a controller. + // CR 110.2 + CR 108.4 (docs/MagicCompRules.txt): only + // permanents have a controller. enters_under: EntersUnderSpec::Default,Same mis-citation recurs at Line 2080-2082 (
d.zonevariant) — apply the same fix there.As per path instructions: "rules-touching code with no verified
CR <number>: <description>annotation, or a CR citation whose rule body does not describe the code" is a finding, and "119 starting-life / 120 damage / 121 draw are adjacent and confused" flags exactly this class of numeric-citation risk.Also applies to: 2080-2085
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/imperative.rs` around lines 2052 - 2057, Correct the rules citations in both `EntersUnderSpec::Default` initializations around the `d.zone` and non-`d.zone` variants: replace the incorrect CR 110.1 reference with citations to CR 110.2 and CR 108.4, which establish controller ownership for permanents and the applicable card types. Keep the existing behavior and annotation wording otherwise unchanged.Source: Path instructions
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/imperative.rs (1)
2325-2357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail-closed
enters_under.unbound_possessor()guard duplicated 5×.The pattern
if let Some(p) = enters_under.unbound_possessor() { return Effect::unimplemented("change_zone_enters_under_anaphor", p.printed_clause()); }followed byenters_under.as_controller_ref()repeats verbatim at Line 2331-2343 (ReturnToBattlefield), Line 2396-2414 (ReturnAllToZone), Line 6802-6819/6843-6849 (lower_put_ast's two arms), and Line 11682-11688 (lower_imperative_family_ast's partition arm). All five call sites are correctly guarded (verified — none is missing the check), but the boilerplate could collapse into one helper, e.g.EntersUnderSpec::resolve_or_unimplemented() -> Result<Option<ControllerRef>, Effect>, callable with?/matchat each site.♻️ Sketch of the consolidation (illustrative, not exhaustive)
- if let Some(p) = enters_under.unbound_possessor() { - return Effect::unimplemented( - "change_zone_enters_under_anaphor", - p.printed_clause(), - ); - } - Effect::ChangeZone { - ... - enters_under: enters_under.as_controller_ref(), + let enters_under = match enters_under.resolve_or_unimplemented("change_zone_enters_under_anaphor") { + Ok(v) => v, + Err(effect) => return effect, + }; + Effect::ChangeZone { + ... + enters_under,Also applies to: 2393-2401, 6787-6849, 11668-11688
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/imperative.rs` around lines 2325 - 2357, Consolidate the repeated fail-closed handling of enters_under.unbound_possessor() into a single EntersUnderSpec helper that returns either the resolved controller reference or the existing Effect::unimplemented result. Update the ReturnToBattlefield, ReturnAllToZone, both lower_put_ast arms, and the lower_imperative_family_ast partition arm to use this helper while preserving the current failure key and printed clause.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 15812-15817: Update both comments associated with
EntersUnderSpec::Default to cite CR 110.2 instead of CR 110.1, while preserving
the existing wording and behavior.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 2052-2057: Correct the rules citations in both
`EntersUnderSpec::Default` initializations around the `d.zone` and non-`d.zone`
variants: replace the incorrect CR 110.1 reference with citations to CR 110.2
and CR 108.4, which establish controller ownership for permanents and the
applicable card types. Keep the existing behavior and annotation wording
otherwise unchanged.
In `@crates/engine/src/parser/oracle_effect/lower.rs`:
- Around line 6754-6768: Update the battlefield return path following
parse_leading_control_clause to consume and apply trailing battlefield riders
such as face down and tapped, including valid rider orderings, before
constructing ReturnDestination. Reuse the existing rider-scanning logic from the
other destination path, preserving the remaining input and setting face_down and
enter_tapped correctly.
- Around line 6639-6662: Update the destination parsing flow around
parse_with_counters_suffix_spanned to inspect its remaining text for a trailing
control clause before finalizing the destination. Use
parse_leading_control_clause to bind the parsed player to dest.control while
preserving existing rider and suffix handling for orderings such as counters
followed by control. Extend the existing regression fixture with an assertion
that dest.control is populated.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 2325-2357: Consolidate the repeated fail-closed handling of
enters_under.unbound_possessor() into a single EntersUnderSpec helper that
returns either the resolved controller reference or the existing
Effect::unimplemented result. Update the ReturnToBattlefield, ReturnAllToZone,
both lower_put_ast arms, and the lower_imperative_family_ast partition arm to
use this helper while preserving the current failure key and printed clause.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae892d00-44e6-4cc1-a1ca-88756500c54c
⛔ Files ignored due to path filters (5)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__aangs_journey_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__analyze_the_pollen_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__boseiju_who_endures_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__case_of_the_stashed_skeleton_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__stoneforge_mystic_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (6)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the new fail-closed representation is bypassed on the SearchDestination assembly path, and the current test suite does not exercise that path.
🔴 Blocker
apply_clause_continuation still patches a pre-existing library-to-battlefield ChangeZone with enters_under.as_controller_ref() before it creates the new Effect::unimplemented node for an unbound anaphor. Evidence: crates/engine/src/parser/oracle_effect/sequence.rs:3727-3740 calls apply_search_destination_to_ability_chain with None; its implementation at crates/engine/src/parser/oracle_effect/sequence.rs:5048-5073 leaves the existing controller untouched while still rewriting destination/tapped. The later unbound branch at crates/engine/src/parser/oracle_effect/sequence.rs:3747-3781 only substitutes the newly appended node and still adds forward_result/Attach when present. That leaves an executable default-controller move alongside the reported change_zone_enters_under_anaphor gap, so the parser reports an honest failure while the chain can still perform the wrong move.
The existing failure-closed test does not cover this production branch: crates/engine/src/parser/oracle_effect/tests.rs:30079-30091 tests a direct return (lower_put_ast), while the affected path is the SearchLibrary continuation. The integration test also explicitly documents that it passes with the pre-fix AST at crates/engine/tests/integration/issue_6691_enters_under_their_control.rs:18-37.
Suggested fix: make EntersUnderSpec::UnboundAnaphor abort or replace the entire search-destination assembly before apply_search_destination_to_ability_chain mutates its predecessor, including the attachment continuation. Add a parser/production-path regression that exercises an unbound search-destination clause and proves no executable ChangeZone or attachment survives.
🟡 Non-blocking
bind_control_clause ends with (p, _) at crates/engine/src/parser/oracle_nom/enters_under.rs:353. The module otherwise deliberately uses exhaustive controller mappings; enumerate the remaining anaphor/antecedent combinations so a future ControlClausePossessor cannot silently become an unexplained gap.
The current parse-diff sticky comment reports six affected cards, while the PR body claims seven and names Endless Whispers as an additional honest gap. Reconcile that claim with the current artifact before re-review; no changed signature in the sticky evidence identifies Endless Whispers. CI is green, but it does not resolve the assembly-path defect above.
Recommendation: request changes. Repair the whole SearchDestination fail-closed assembly and cover that exact path before re-review.
|
Current-head disposition — rebase required before re-review. I reproduced a textual conflict between this head and current Please rebase this head onto current |
0266dab to
dde4f83
Compare
|
Current-head review complete — held for external evidence. The prior SearchDestination blocker is resolved on head dde4f83: sequence.rs now replaces the whole preinstalled search assembly before any default-controller move or attachment can survive, and the added production-path parser regression asserts that no ChangeZone or Attach remains for an unbound clause. Required Rust lint/tests/card-data checks and the current-head parse-diff artifact are still running; CodeRabbit is also pending. This is a non-terminal hold only: once those current-head results settle, maintainer review will recheck the artifact/reviewer findings and then approve or request changes. No merge-queue action has been taken. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_effect/imperative.rs (1)
12220-12243: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the unbound control clause before attaching the sub-ability.
lower_targeted_action_astreturnsEffect::unimplementedwhenenters_under.unbound_possessor()isSome(line 2340). This arm ignores that outcome and still installsEffect::Attach { attachment: SelfRef, target: host }asclause.sub_ability. For the form"return … to the battlefield under their control attached to <host>", the zone change becomes a coverage gap while the attach still resolves. The source then attaches to the host although the permanent never entered the battlefield.Every other lowering site in this change guards first — lines 2340, 2405, 6840, 6874, and 11713. Apply the same guard here so the whole clause fails closed.
🛡️ Proposed fail-closed guard
)) => { + // CR 110.2a (docs/MagicCompRules.txt:618): fail closed BEFORE the + // Attach sub-ability is built. A gapped ChangeZone paired with a + // live Attach would attach the source to the host although the + // permanent never entered the battlefield. + if let Some(p) = enters_under.unbound_possessor() { + return parsed_clause(Effect::unimplemented( + "change_zone_enters_under_anaphor", + p.printed_clause(), + )); + } let mut clause = parsed_clause(lower_targeted_action_ast( TargetedImperativeAst::ReturnToBattlefield {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/imperative.rs` around lines 12220 - 12243, Guard the result of lower_targeted_action_ast in the ReturnToBattlefield arm before assigning the Attach sub-ability: when enters_under.unbound_possessor() is Some, preserve the unimplemented clause and do not install Effect::Attach. Match the existing fail-closed handling used by the other lowering sites, while retaining the current SelfRef attachment behavior for bound control clauses.
🧹 Nitpick comments (1)
crates/engine/tests/integration/issue_6691_enters_under_their_control.rs (1)
95-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the parsed effect instead of a
Debugsubstring.The guard formats the whole face with
{face:?}and searches forParentTargetOwner. Any unrelated field or ability that stringifies that variant satisfies the guard, so it can pass for the wrong reason. The guard also does not pin which field carries the binding.Walk the parsed abilities and match the shape directly, for example
Effect::ChangeZone { destination: Zone::Battlefield, enters_under: Some(ControllerRef::ParentTargetOwner), .. }. The guard then fails only for the intended reason and survives a variant rename as a compile error rather than a silent weakening.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/issue_6691_enters_under_their_control.rs` around lines 95 - 106, Replace the Debug-string assertion in the Jailbreak fixture guard with a direct match over its parsed abilities, identifying an Effect::ChangeZone whose destination is Zone::Battlefield and whose enters_under is Some(ControllerRef::ParentTargetOwner). Keep the assertion scoped to that intended parsed effect so unrelated fields cannot satisfy it.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 12220-12243: Guard the result of lower_targeted_action_ast in the
ReturnToBattlefield arm before assigning the Attach sub-ability: when
enters_under.unbound_possessor() is Some, preserve the unimplemented clause and
do not install Effect::Attach. Match the existing fail-closed handling used by
the other lowering sites, while retaining the current SelfRef attachment
behavior for bound control clauses.
---
Nitpick comments:
In `@crates/engine/tests/integration/issue_6691_enters_under_their_control.rs`:
- Around line 95-106: Replace the Debug-string assertion in the Jailbreak
fixture guard with a direct match over its parsed abilities, identifying an
Effect::ChangeZone whose destination is Zone::Battlefield and whose enters_under
is Some(ControllerRef::ParentTargetOwner). Keep the assertion scoped to that
intended parsed effect so unrelated fields cannot satisfy it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53f719dd-ab49-4e0a-b946-ee1476ee9d19
⛔ Files ignored due to path filters (5)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__aangs_journey_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__analyze_the_pollen_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__boseiju_who_endures_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__case_of_the_stashed_skeleton_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__stoneforge_mystic_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (11)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_nom/enters_under.rscrates/engine/src/parser/oracle_nom/mod.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/issue_6691_enters_under_their_control.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/engine/src/parser/oracle_nom/mod.rs
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_effect/mod.rs
- crates/engine/src/parser/oracle_effect/tests.rs
- crates/engine/src/parser/oracle_effect/sequence.rs
- crates/engine/src/parser/oracle_ir/ast.rs
- crates/engine/src/parser/oracle_nom/enters_under.rs
|
Current-head maintainer hold for Please let the current Rust/card-data CI and a current-head |
|
Maintainer follow-up on current head |
|
Correction to the abbreviated/full SHA in the prior hold: the current head is |
…s#6691) The Oracle parser recognised only "under your control" and the owner-stating forms. "under their control" / "under that player's control" fell through the empty-tag arm and were silently dropped, so permanents entered under the wrong controller while the cards reported supported: true, gap_count: 0 — a silent wrong-game-result class. Collapse four independently-implemented copies of the `under <possessor> control` grammar into one nom combinator module (oracle_nom/enters_under.rs) and bind the anaphor from typed parse products only: N1 — the moved object's own FilterProp::Owned { controller != You } (CR 400.1 + 400.3 + 404.1 + 108.3: a card in a graveyard is in its owner's graveyard) -> ControllerRef::ParentTargetOwner N2 — ctx.relative_player_scope, mapped through an exhaustive wildcard-free table that admits only ScopedPlayer, ChosenPlayer and TriggeringPlayer Everything else fails closed to Effect::unimplemented rather than silently defaulting, so a clause the parser cannot bind becomes an honest coverage gap instead of a wrong controller. EntersUnderSpec carries that third state through the AST to every lowering site. Fixes Jailbreak (ParentTargetOwner) and Gerrymandering (ScopedPlayer) and their templates; 933 "your"/owner-form cards keep byte-identical behaviour through the single grammar. Addresses phase-rs#6691; does not close it — see the PR body for the residue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Nr9Cg6eBZfBod5Twb6Vyi
…h + scope correction Review found the integration test passed with the fix reverted. Two causes, both fixed here: 1. The surgically-added `jailbreak` fixture entry was copied from a PRE-FIX `card-data.json`, so it carried no `enters_under` key. Since the field is `skip_serializing_if = "Option::is_none"`, it deserialized as the unfixed AST and the test measured old behaviour while appearing to pass. The entry is regenerated from post-fix card data (+0 keys, only `jailbreak` changed) and a fixture-integrity guard now fails loudly if it ever goes stale again. 2. The test claimed to be revert-failing. It is not, and that was MEASURED, not assumed: with `enters_under` stripped it still passes, because a graveyard → battlefield move already resolves the new object's controller to its OWNER, which for Jailbreak is exactly the player the clause names. Staging a divergent controller does not separate them either — the move reassigns it. The docstring now states this plainly and points at the discriminating test. The discriminating test is the parser-level assertion `jailbreak_enters_under_their_control_binds_to_the_owner`, verified by temporarily binding `Default` instead of `ParentTargetOwner`: it FAILS on revert and passes when restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Nr9Cg6eBZfBod5Twb6Vyi
5ce5045 to
29bc00e
Compare
|
Current-head hold for Fresh implementation review is clean for the formerly blocked paths: unbound control clauses now replace the complete This is not an approval yet. Rust lint and both Rust test shards remain in progress, and the only parse-diff artifact is timestamped |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for the current head: the typed enters_under authority, complete fail-closed SearchDestination/attached-return handling, card-level parse delta, and discriminating runtime coverage have been re-reviewed clean.
3 participants
Summary
Addresses #6691 for the
FilterProp::Ownedtemplate and theplayer_scopefan-out template; does not close it — residue enumerated below.The parser recognised only
"under your control"and the owner-stating forms."under their control"/"under that player's control"fell through the empty-tag arm of the alternation atoracle_effect/lower.rs:6705and were silently dropped, so permanents entered under the wrong controller while the cards reportedsupported: true, gap_count: 0— a silent wrong-game-result class.This collapses four independently-implemented copies of the
under <possessor> controlgrammar into one nom combinator module and binds the anaphor from typed parse products only, never a text scan or a lowered-tree walk:FilterProp::Owned { controller != You }→ControllerRef::ParentTargetOwner. Licensed by CR 400.1 + 400.3 + 404.1 (a card in a graveyard is in its owner's graveyard) + CR 108.3.ctx.relative_player_scope, mapped through an exhaustive, wildcard-free table admitting onlyScopedPlayer,ChosenPlayer,TriggeringPlayer.TargetPlayer/TargetOpponentare refused by name: two incompatible provenances, andTargetPlayerresolves to the first player target, which would put every land under one player's control on a multi-target cast (Turtle Tracks).Anything unbindable fails closed to
Effect::unimplemented, so a clause the parser cannot bind becomes an honest coverage gap instead of a wrong controller.EntersUnderSpeccarries that third state — whichOption<ControllerRef>cannot express, and whose absence is the bug — through the AST to every lowering site.Files changed
crates/engine/src/parser/oracle_nom/enters_under.rs(new)crates/engine/src/parser/oracle_nom/mod.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_effect/{lower,mod,imperative,sequence,tests}.rscrates/engine/src/parser/oracle_ir/snapshots/*.snap(2)crates/engine/tests/integration/issue_6691_enters_under_their_control.rs(new),tests/integration/main.rs,tests/fixtures/integration_cards.jsonCR references
CR 110.2a @618 (authorizing) · 110.2 @616 · 110.1 @614 · 608.2c @2793 · 608.2k @2814 · 608.2e @2798 · 400.1 @1933 · 400.3 @1937 · 404.1 @2030 · 108.3 @564 · 109.4 @594 · 109.5 @610 · 115.10 @886 · 102.2/102.3 @252/254 · 603.2 @2561 · 614.1c @3060 · 508.4 @2309 · 708.3 @5707
All grep-verified in
docs/MagicCompRules.txtbefore being written; the diff gate returns zero UNVERIFIED.Measured impact (full-pool diff, not predicted)
Regenerated
card-data.jsonbefore/after. Exactly 7 cards move; zero others change:ParentTargetOwnerScopedPlayerScopedPlayer933
your/owner-form cards (558+316+27+20+6+6) keep byte-identical behaviour while flowing through a single grammar. The 13 deferred possessor forms become a one-alt-arm extension instead of four parallel edits.Coverage moves down by 4 by design: those cards were reporting
gap_count: 0on apriority:p2-wrong-game-resultclause. An honest gap beats a wrong controller.Implementation method
Method: /engine-implementer — plan →
/review-engine-planx2 (both returned blockers; both resolved) → implement →/review-impl(returned a blocker; resolved, see Validation Failures).Track
Developer
LLM
Model: claude-opus-5
Thinking: high
Verification
Required checks ran clean.
Gate A output below is for the current committed head.
Both anchors cite existing analogous code at the same seam.
Final
review-implcould not be re-run after the last commit — see Validation Failures.cargo fmt --all— cleancargo check -p engine --lib— 0 errors, 0 warningscargo test -p engine --lib parser::— 8678 passed; 0 failedcargo test -p engine --test integration— 4164 passed; 0 failedcargo clippy -p engine --all-targets— no warnings./scripts/gen-card-data.sh— regenerated; full-pool diff aboveManual grep for the ungated blind spots (
.rfind/.split/.split_once/.contains() in added parser lines — one hit, a test assertion on a debug dump attests.rs:29926, not dispatch. Noallow-noncombinatoradded anywhere.Revert-sensitivity proven, not asserted. Temporarily binding
EntersUnderSpec::Defaultinstead ofOverride(ParentTargetOwner)makesjailbreak_enters_under_their_control_binds_to_the_ownerFAIL; restoring makes it pass.Gate A
Anchored on
crates/engine/src/parser/oracle_effect/lower.rs:6705— the existingenters_underalt()binding" under your control"->Some(ControllerRef::You)and returningNonefor the owner forms. The exact seam the new grammar replaces; it already carried its ownCR 110.2aannotation.crates/engine/src/parser/oracle_effect/lower.rs:6641— the sibling destination-table carrierenters_under: enters_under_you.then_some(ControllerRef::You), showing how the same controller override is threaded through the phrase-table path.Claimed parse impact
Residue — why this does not close #6691
supported: true, gap_count: 0withenters_under: "You"on its "and the rest onto the battlefield tapped under their control" leg — parser:under their controlbinding unparsed — permanents enter under the wrong controller (CR 110.2a) #6691's exact failure mode surviving this fix, because that leg is produced by a dig/reveal path this change does not rewire (sequence.rs:5468/5560/6868/6930).is_static_pattern()at priority 7 (its text contains "can't block") and never reaches these seams — a separate line-classifier defect.// FOLLOW-UPcomments: the four dig/reveal legs,imperative.rs:390-398,imperative.rs:6440, andoracle_trigger.rs:9597(the trigger-condition layer, CR 603.2 — deliberately out of scope, and the home of the 7 Trap/condition cards that must not be touched).under an opponent's controletc.) are explicit NPs, not anaphors, and are unchanged.Validation Failures
The final
review-implpass could not be re-run against the current head2b25293. An independentreview-impldid run against the preceding head and returned one BLOCKER plus five lower findings; the BLOCKER is fixed in this branch (commit 2 — the integration test passed with the fix reverted, because the fixture entry was stale and the runtime default coincides with the correct answer for Jailbreak). The environment then exhausted its model budget, so the confirming re-review did not run.Recorded but not fixed, for maintainer triage:
apply_search_destination_to_ability_chainis called withNoneon an unbound anaphor so the destination/tapped patches survive (deliberately — skipping the call would drop them), but a predecessorChangeZonecan still execute with the default controller while only the newly-pushed def becomesUnimplemented.fold_control_clauseswidens the pattern set over an unchanged span. The span is byte-identical to the literal it replaces, but the fold now also matches the anaphor anywhere in it. No printed card currently pairs a conditional"...enter the battlefield under their control this turn"with a battlefield put on the same normalized line, so the guard is incidental rather than structural.enters_under: Nonemeans "keep current controller".lower->textbyte offsets across a clause containing'and U+2019. Both are length-preserving underto_lowercase(so the flagged risk is not live), butTextPairwould make it structural.Also not run:
cargo coverage,cargo semantic-audit,ordering_parity_sweep,scripts/coverage-regression-check.sh. The full-poolcard-data.jsondiff above is the substantive evidence in their place; predicted bucketparser_regress(non-fatal), net supported delta -4, which CI will confirm.CI Failures
None observed locally.
Generated by Claude Code
Summary by CodeRabbit